Smooth ramping of ATR and Torque Tilt #2
Conversation
surfdado
force-pushed
the
smooth_ramping
branch
2 times, most recently
from
d224d82 to
7005dda
Compare
Following the example of inputtilt, ATR setpoints are now ramped slower when near the current setpoint. This allows for much higher ramping speeds without causing a jerking effect. Instead of using 5deg/sec and 3deg/sec one can now easily run 20 and 12 or even more without any perceived downsides. Feature: Smoothened/faster setpoint ramping for ATR Signed-off-by: Dado Mista <[email protected]>
Following the example of inputtilt, TT setpoints are now ramped slower when near the current setpoint. This allows for much higher ramping speeds without causing a jerking effect. Instead of using 5deg/sec and 3deg/sec one can now easily run 20 and 12 or even more without any perceived downsides. Feature: Smoothened/faster setpoint ramping for Torque Tilt Signed-off-by: Dado Mista <[email protected]>
src/torque_tilt.c
Outdated
| } | ||
| } else { | ||
| rate_limitf(&tt->offset, target_offset, step_size); | ||
| } |
There was a problem hiding this comment.
Just having a very quick look now, I'll want this extracted into a function, not copy-pasted. I think there are also multiple ways to do the smoothing, there might be a better way that'd allow simpler (needing less context) implementation in a function. At least those were the thoughts I had when looking at the original PR...
Signed-off-by: Dado Mista <[email protected]>
|
Factored it out into a function now (without changing the logic). Haven't done any test rides with it yet to see if functionality got affected... |
|
The way I did something similar was by having targets already smooth, multiplying target_diff by a "Responsiveness" parameter (let's say 15) to produce deg/s, which gets limited by "Max Speed". Then multiply by dt to get the step. So just a rate limited P control. I like how simple yet user friendly it is (for example changing Max Speed doesn't affect near-target behavior). It's been working great for me, but tbf I haven't tried the proposed changes, so maybe I'm missing out on something. |
|
Where are we on this? People are asking for this feature and I'm tired of sending them custom Refloat packages... |
|
I'm sorry for the delay, I wanted to test this, but I don't have that much experience with ATR. I'll get to it this weekend. |
| ((1 - smoothing_factor) * *ramped_step); | ||
| // Linearly ramped down step size is provided as minimum to prevent overshoot | ||
| float centering_step = fminf(fabsf(*ramped_step), fabsf(tiltback_target_diff / 2) * step) * | ||
| sign(tiltback_target_diff); |
There was a problem hiding this comment.
I'm not really great at math, so I may totally be missing the point, but I find this computation overly complicated and ultimately flawed.
The step here is no longer the step in itself, it's a multiplier for the tiltback_target_diff and the step is thus a linear function of the diff. The way I understand it you are filtering towards the real step (ramped_step) if outside the window and you're filtering towards half of the real step when within the window.
The line above is what I think is not working really well, consider the case when you've just entered the smooth_center_window after you've filtered almost towards step * tiltback_target_diff, so that will be the value of *ramped_step. centering_step, though, after going through the fminf(), will be fabsf(tiltback_target_diff / 2) * step, which is almost half of that, so that transition won't be smooth at all. Or am I wrong on this?
I don't dispute it's working in a way (not sure why the above non-continuity doesn't noticeably manifest), but I want the math in the package to be solid. I think this is supposed to do in essence what @aeraglyx described in #2 (comment), except his solution I think is (to the extent I can make that claim, again, I'm not really too good at math) mathematically correct. There are likely more ways of doing this too, e.g. I was thinking of maybe an exponential function (while this one is linear), etc.
There was a problem hiding this comment.
I have to admit that I simply copy pasted this code without questioning it from the Input-tilt implementation. Now that you forced me to look into it I have to say that something does look fishy about that code, good job for catching that.
Before further investigating I double-checked that I didn't introduce the problem during refactoring. Thus the problem should also affect the original input tilt implementation.
I think the problem starts when ramped_step_size is updated for the smooth center window. If the smooth center window is made large enough (>> 2), it appears as if ramped_step_size could actually temporarily increase instead of decreasing, since "tiltback_target_diff / 2" can be larger than 1.
I haven't had time yet to look into a fix but I figured I owed you a response acknowledging the problem you found.
|
Very excited to see what you have in mind after reading through Discord, @lukash! I've been toying with this too, might as well share some findings:
Here's a plot comparing a few options on some generated noisy data.
I'm probably risking being super annoying considering that you teased about posting plots, that's not my intention 😅 And not sure how useful it is, but I found out that this form is pretty much identical to a 2nd order IIR + rate limiting. Essentially what my previous comment suggested but with additional speed smoothing (2nd order requires 4x less filtering per "speed EMA", 3rd order 6x, not sure what's up with that). I don't have this in code, sorry.
|
This feature has been tested by a handful of riders for the past few months.
Following the example of inputtilt where ramping rates of over 50deg/sec are used,
we're doing the same for ATR and Torque Tilt now.
Setpoints are ramped slower when near the current setpoint. This allows for much higher
ramping speeds without causing a jerking effect.
Instead of using 5deg/sec and 3deg/sec one can now easily run 20 and 12
or even more without any perceived downsides.